Skip to content

fix(vm): guard negative forward jump offsets #861

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

fix(vm): guard negative forward jump offsets #861

wants to merge 1 commit into from

Conversation

@thevilledev
Copy link
Contributor

@thevilledev thevilledev commented Nov 15, 2025 •
edited
Loading

From #860 (comment)

Disallow negative offsets for forward jump opcodes in the VM. A crafted Program or fuzzed bytecode could pass negative arguments for OpJump and the conditional jump variants, causing unsafe control flow. Now these opcodes panic with a clear error when given a negative offset. Worth noting that the compiler never emits negative offsets, for example see in compiler.patchJump.

The test times out against master branch as it's basically an infinite loop.

Bench test showing not much of a difference:

goos: linux
goarch: amd64
pkg: github.com/expr-lang/expr
cpu: AMD EPYC 9575F 64-Core Processor
                            │   before    │              after-jump               │
                            │   sec/op    │    sec/op      vs base                │
_expr-32                      47.99n ± 3%    45.76n ±  0%   -4.64% (p=0.000 n=10)
_expr_eval-32                 4.173µ ± 1%    4.100µ ±  1%   -1.75% (p=0.000 n=10)
_expr_reuseVm-32              30.41n ± 5%    30.43n ±  2%        ~ (p=0.699 n=10)
_len-32                       29.98n ± 1%    29.92n ±  1%   -0.20% (p=0.050 n=10)
_filter-32                    34.79µ ± 1%    32.08µ ±  3%   -7.79% (p=0.000 n=10)
_filterLen-32                 29.33µ ± 4%    28.65µ ±  0%   -2.31% (p=0.000 n=10)
_filterFirst-32               318.6n ± 1%    298.8n ±  1%   -6.20% (p=0.000 n=10)
_filterLast-32                549.2n ± 1%    548.8n ±  1%        ~ (p=0.644 n=10)
_filterMap-32                 3.764µ ± 1%    3.704µ ±  1%   -1.59% (p=0.000 n=10)
_arrayIndex-32                51.59n ± 9%    46.25n ±  2%  -10.34% (p=0.002 n=10)
_envStruct-32                 33.23n ± 1%    32.01n ±  1%   -3.66% (p=0.000 n=10)
_envMap-32                    37.17n ± 1%    36.20n ±  0%   -2.60% (p=0.000 n=10)
_callFunc-32                  234.0n ± 1%    232.5n ±  1%   -0.66% (p=0.033 n=10)
_callMethod-32                248.3n ± 1%    248.7n ±  1%        ~ (p=0.839 n=10)
_callField-32                 49.24n ± 1%    48.90n ± 17%        ~ (p=0.516 n=10)
_callFast-32                  50.42n ± 5%    49.58n ±  0%        ~ (p=0.052 n=10)
_callConstExpr-32             19.52n ± 3%    21.53n ±  0%  +10.27% (p=0.000 n=10)
_largeStructAccess-32         94.00n ± 1%    95.96n ±  1%   +2.08% (p=0.002 n=10)
_largeNestedStructAccess-32   99.37n ± 1%   100.85n ±  1%   +1.49% (p=0.001 n=10)
_largeNestedArrayAccess-32    460.0µ ± 1%    469.1µ ±  1%   +1.98% (p=0.000 n=10)
_sort-32                      2.636µ ± 2%    2.719µ ±  1%   +3.13% (p=0.001 n=10)
_sortBy-32                    6.575µ ± 1%    6.891µ ±  0%   +4.81% (p=0.000 n=10)
_groupBy-32                   7.182µ ± 1%    7.745µ ±  0%   +7.84% (p=0.000 n=10)
_reduce-32                    3.326µ ± 2%    3.504µ ±  1%   +5.37% (p=0.000 n=10)
geomean                       491.3n         489.4n         -0.40%

@thevilledev
fix(vm): guard negative forward jump offsets
b875d6c 
Disallow negative offsets for forward jump opcodes in the VM.
The compiler only ever emits non-negative offsets, but a crafted
Program or fuzzed bytecode could pass negative arguments for
OpJump and the conditional jump variants, causing unsafe control
flow. Now these opcodes panic with a clear error when given a
negative offset.

Signed-off-by: Ville Vesilehto <ville@vesilehto.fi>
@antonmedv
Copy link
Member

antonmedv commented Nov 15, 2025

Those benchmarks unfortunately inconclusive

@thevilledev
Copy link
Contributor Author

thevilledev commented Nov 15, 2025 •
edited
Loading

I also tried it with pathological cases, like deep nested conditionals:

exprStr := `
  ((a > 0 && b > 0) || (a > 1 && b > 1) || (a > 2 && b > 2) || (a > 3 && b > 3) || (a > 4 && b > 4)) &&
  ((c < 10 || c < 9 || c < 8 || c < 7 || c < 6 || c < 5)) &&
  (a == 1 ? (b == 2 ? c == 3 : b == 2 || c == 3) : (a == 1 || b == 2 || c == 3))
`

But of course the compiler is guaranteed to push non-negative jumps. I disassembled the vm.Run to see what it does under the hood. On ARM64 the if arg < 0 branch translates to:

  vm.go:179             0x1001f17e4             b7fa6fa2                TBNZ $63, R2, 4989(PC)

Meaning "test bit and branch if non-zero". Branch prediction with compiler only emitting non-negative numbers makes this very lightweight of a check. So IMO the impact of this change is almost a no-op.

On x86-64:

  vm.go:179             0x629720                4885ff                          TESTQ DI, DI
  vm.go:179             0x629723                0f8c7c580000                    JL 0x62efa5

Checks the sign of arg first, which compiler guarantees to be non-negative. The "jump if less" that follows is a conditional branch that I'd guess is ~never taken due to branch prediction. The sign check is one ALU op.

Happy to hear other ideas/concerns.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@thevilledev @antonmedv